Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[web-components]Fix document capture hide attribution #370

Merged
merged 2 commits into from
Dec 11, 2024
Merged

[web-components]Fix document capture hide attribution #370

merged 2 commits into from
Dec 11, 2024
+263 −25

Conversation

ayinloya
Copy link
Collaborator

This adds a fix for attribution not hidden on document capture screens even when the attribute hide-attribution is supplied through the smart-camera-web tag.

@ayinloya ayinloya self-assigned this Dec 10, 2024
@prfectionist
Copy link

prfectionist bot commented Dec 10, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Duplication
The hideAttribution attribute is being passed multiple times to similar components. Consider extracting this into a reusable template or helper function to reduce duplication.

@ayinloya
Copy link
Collaborator Author

https://app.shortcut.com/smileid/story/14449/fix-javascript-sdk-new-component-issues

@prfectionist
Copy link

prfectionist bot commented Dec 10, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Reduce test code duplication by extracting common verification patterns into reusable commands

Extract repeated test steps into reusable custom Cypress commands to reduce code
duplication and improve test maintainability.

packages/web-components/cypress/e2e/smart-camera-web-attribution.cy.js [5-10]

-cy.get('smart-camera-web')
-  .shadow()
-  .find('selfie-capture-instructions')
-  .shadow()
-  .find('powered-by-smile-id')
-  .should('be.visible');
+cy.checkAttributionVisibility('selfie-capture-instructions', true);
Suggestion importance[1-10]: 7

Why: Creating reusable Cypress commands would significantly reduce code duplication across the test file and make tests more maintainable, as similar attribution checks are repeated multiple times.

7
Use test hooks to centralize common test setup and reduce code duplication

Consider using beforeEach() to set up common test conditions like cy.clock() to
avoid repetition and ensure consistent test setup.

packages/web-components/cypress/e2e/smart-camera-web-attribution.cy.js [2-4]

-it('shows attribution by default', () => {
+beforeEach(() => {
   cy.clock();
   cy.visit('/smart-camera-web');
+});
 
+it('shows attribution by default', () => {
+
Suggestion importance[1-10]: 6

Why: Using beforeEach() would eliminate repeated setup code across test cases and ensure consistent test initialization, improving test maintainability.

6
Maintainability
Improve template literal formatting for better code readability and maintenance

Consider using template literals for better readability and maintainability when
concatenating multiple attributes. Use line breaks and indentation to improve code
structure.

packages/web-components/components/document/src/DocumentCaptureScreens.js [37-40]

-<document-capture-instructions theme-color='${this.themeColor}' id='document-capture-instructions-front' ${this.title}
-${this.documentCaptureModes} ${this.showNavigation} ${this.hideInstructions ? 'hidden' : ''}
-${this.hideAttribution}
+<document-capture-instructions
+  theme-color='${this.themeColor}'
+  id='document-capture-instructions-front'
+  ${this.title}
+  ${this.documentCaptureModes}
+  ${this.showNavigation}
+  ${this.hideInstructions ? 'hidden' : ''}
+  ${this.hideAttribution}
 ></document-capture-instructions>
Suggestion importance[1-10]: 5

Why: The suggestion improves code readability by breaking down long template literals into multiple lines with proper indentation, making the code more maintainable and easier to understand.

5

@ayinloya ayinloya merged commit 4516edc into main Dec 11, 2024
12 of 13 checks passed
@ayinloya ayinloya deleted the fix-document-capture-hide-attribution branch December 11, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beaglebets beaglebets beaglebets approved these changes

@beastawakens beastawakens Awaiting requested review from beastawakens

@solnsubuga solnsubuga Awaiting requested review from solnsubuga

Assignees

@ayinloya ayinloya

Labels
Review effort [1-5]: 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ayinloya @beaglebets